sql, redis: call tls.checkServerIdentity, honor tls.servername, send SNI from RedisClient - #42054

…eturn value (#42987)

### Problem

- A `tls.checkServerIdentity` function replaces Bun's hostname check,
and only an Error return value rejects the certificate
(`run_check_server_identity`,
`src/runtime/webcore/fetch/FetchTasklet.rs:1213`). A Promise approves
it, so an `async` wrapper of `tls.checkServerIdentity` accepts any host:
on Bun 1.4.3-canary a `localhost` certificate, dialled as `127.0.0.1`,
resolves.
- `fetch()` ignores a `checkServerIdentity` that is not a function.
`parse_tls` (`src/runtime/webcore/fetch/FetchSession.rs:62`) sets it
aside, and only the `Bun.FetchSession` constructor throws for it
(#42937).
- Node and Bun's `node:https` fail the connection in these cases.

### Fix

- Every truthy return value fails the request, as in Node: an object is
the rejection reason, a Promise or a primitive gives
`ERR_INVALID_RETURN_VALUE`. A falsy value, a thrown value and
`rejectUnauthorized: false` behave as before.
- `parse_tls` throws `ERR_INVALID_ARG_TYPE` for a value that is not a
function, so `fetch()` rejects as the constructor throws. The
`unusable_check_server_identity` field of #42937 is gone. `null` and
`undefined` mean no function.
- **Behaviour change on a released API.** A function that returns
`true`, `hosts.push(host)` or a Promise now gets a rejected `fetch()`. A
maintainer must decide which release carries this.
- Verified: `test/js/web/fetch/fetch.tls.test.ts` (5 new tests fail on a
debug build of main, all 46 pass with the change). Other suites: see
Notes.

### Background

- `tls.checkServerIdentity(hostname, cert)` is a Bun extension to
`fetch()`, named after the `node:tls` option. Bun calls it after the
certificate chain verifies, and sends the request only after approval.
- `Bun.FetchSession` (#42692, in no release) holds connection settings,
this option included, for the `fetch()` calls that name it.
- `ERR_INVALID_RETURN_VALUE` is the Node error code for a callback that
returns the wrong type.

<details><summary>Notes</summary>

**Source and reach.** No user reported this. A sweep of the
`Bun.FetchSession` option bag found it during the work on #42937. That
PR left a `fetch()` request unchanged on purpose and said that the
request path needs its own PR, because it is a released surface. This is
that PR. In this repository, every function that a test passes to
`fetch()` or to `Bun.FetchSession` returns `undefined` or an Error, and
no module in `src/js` passes the option to `fetch()`. The type in
`globals.d.ts` already excludes every input that now fails. So the case
for the change is the severity when it happens, not the number of users.

**The rule for a `tls.checkServerIdentity` option, for a maintainer to
accept or change once.**

| Input | Result |
| --- | --- |
| option is `undefined` or `null` | no function, Bun's own hostname
check runs |
| option is a function | Bun calls it after the chain verifies, in place
of its own hostname check |
| option is any other value | `ERR_INVALID_ARG_TYPE` |
| function returns a falsy value | approved |
| function returns an object (an Error, a `DOMException`, any other) |
the request fails with that object |
| function throws | the request fails with the thrown value |
| function returns a Promise or a truthy primitive | the request fails
with `ERR_INVALID_RETURN_VALUE` |
| `rejectUnauthorized: false` | the function runs, Bun ignores the
result |

**Repro.** An HTTPS server with the `tls` pair of `test/harness`, `ca`
set to that certificate, one request for each function. `fetch` is Bun
1.4.3-canary and a debug build of main. `https.get` is Node v26.3.0.
Bun's `node:https` gives the same result in every row but the `throws`
row.

| `checkServerIdentity` | `fetch`, before | `fetch`, after | `https.get`
|
| --- | --- | --- | --- |
| returns `undefined`, `null`, `false`, `0`, `""` | resolves | resolves
| resolves |
| returns an Error | rejects with it | rejects with it | `error`: it |
| throws | rejects with the value | rejects with the value | uncaught
exception |
| returns `true` | resolves | `ERR_INVALID_RETURN_VALUE` | `error`:
`true` |
| returns `"pin mismatch"` | resolves | `ERR_INVALID_RETURN_VALUE` |
`error`: the string |
| returns `{}` or a `DOMException` | resolves | rejects with it |
`error`: the object |
| `async`, resolves to an Error | resolves | `ERR_INVALID_RETURN_VALUE`
| `error`: the Promise |
| is `"yes"`, `true`, `1`, `{}`, `false` | resolves, no check |
`ERR_INVALID_ARG_TYPE` | throws `ERR_INVALID_ARG_TYPE` |
| is `null` or `undefined` | resolves, no check | resolves, no check |
throws `ERR_INVALID_ARG_TYPE` |

**Node source.** `onConnectSecure` assigns `verifyError =
options.checkServerIdentity(hostname, cert)` and then tests `if
(verifyError)`:
https://github.com/nodejs/node/blob/v26.3.0/lib/internal/tls/wrap.js#L1671-L1688
. `tls.connect` calls `validateFunction(options.checkServerIdentity,
"options.checkServerIdentity")`. Bun's `node:tls` has both rules already
(`src/js/node/net.ts`, `src/js/node/tls.ts`).

**Why a TypeError for a Promise and a primitive.** Node destroys the
socket with the returned value, so the `error` event carries `true` or a
Promise. `fetch()` would then reject with `true`. Error handlers read
`e.message`, and the two likely mistakes (`async`, `return true`) need
an explanation, not an echo. The message follows the Node template:
`Expected undefined or an Error to be returned from the
"tls.checkServerIdentity" function but got an instance of Promise.` An
object is different. An Error need not be an `ErrorInstance` cell: a
`DOMException` and a `util.inherits()` error are objects that inherit
from `Error.prototype`. So every object is the rejection reason as it
is, as in Node, and the TypeError cannot say "not an Error" to a value
that is one. If a maintainer prefers the Node rule for every value, the
change is to return `check_result` in place of the TypeError.

**What this does not close.**

- A falsy value still approves, as in Node: `return mismatch && new
Error("pin")` returns `false` on success. So a function that answers
with a boolean (`return cert.fingerprint256 === PIN`) now fails every
request that it approves, and still approves a mismatch. The docs and
the JSDoc now say that `false` approves, and that the type allows only
`undefined` and `Error` for that reason. The type is the one that
`@types/node` declares for the `node:tls` option. Two `@ts-expect-error`
lines in `test/integration/bun-types/fixture/fetch.ts` hold `() =>
false` and an `async` function as type errors.
- `new Request(url, { tls })` drops the whole `tls` option, as the JSDoc
of `BunFetchRequestInit` says. A function given there does not run and
is not validated.
- An unhandled rejection from an `async` function that rejects is
reported as before.
- With `rejectUnauthorized: false` Bun ignores a function that throws,
as it ignores every result. #42692 set this rule and holds it in a test
(`"thrown, not enforced"`). Node reports such a throw as an uncaught
exception.
- `parse_tls` also has a branch that reads a numeric
`rejectUnauthorized`. `SSLConfig::from_js` throws for a number
(`TLSOptions.rejectUnauthorized must be a boolean`), so only a getter
that answers differently on the second read reaches that branch. This PR
leaves it alone.

**`null`.** `fetch()` reads `null` as no function, and `undefined` too,
as the `Bun.FetchSession` constructor does. Node throws for both,
because `tls.connect` always has a default function to replace.

**Precedent for caution.** Bun 1.3.4 made `fetch()` throw for a `proxy`
object without `url`, and users of npm-registry-fetch broke (#25413),
because Bun's `node:http` client forwarded the `proxy` property of the
agent to `fetch()`. #25414 made `fetch()` ignore such a value again. No
module in `src/js` passes `tls.checkServerIdentity` to `fetch()` today,
and the `node:http` client no longer uses `fetch()`. In the other
direction, `proxy: true` on a request throws `ERR_INVALID_ARG_TYPE`
since #42692, with no flag.

**Relation to other PRs.**

- #42937 (merged) made the `Bun.FetchSession` constructor throw for a
`checkServerIdentity` that is not a function, through a
`TlsOption.unusable_check_server_identity` field that a request ignored.
This PR throws in `parse_tls` for both callers and deletes that field
and the check in the constructor. The constructor test of #42937 in
`fetch-session.test.ts` passes unchanged.
- #42054 (sql, redis) and #41648 (WebSocket) add a `checkServerIdentity`
option with the old rule (only an Error fails), modelled on `fetch()`.
They need the rule that a maintainer accepts here. A shared helper for
the rule belongs to the first of them that lands, because main has one
native caller today.
- #35609 forwards the `checkServerIdentity` of a node-fetch agent to
`fetch()`. Such a function follows the Node contract, so it needs the
Node rule for its return value.
- A function that throws under Bun's `node:https` is reported as an
uncaught exception and the request continues. That path is
`src/js/node/net.ts`, which #32824 changes. This PR does not touch it.

**Suggested release note.** `fetch()` now rejects when
`tls.checkServerIdentity` is not a function, or when it returns a truthy
value that is not an Error, for example the Promise of an `async`
function. Before, Bun ignored such an option and approved the
certificate for such a return value.

**Self-review.** 13 concerns raised, 10 addressed. Most asked for facts
in this description: the repro that leads the Problem section (now also
a test), the reach, the rule table, the order relative to #42937, the
release note, and the notes about `new Request()` and the numeric
`rejectUnauthorized` branch. Not done:

- Move the two rules into a shared helper in `bun_jsc` (asked twice).
Main has one native caller, and REVIEW.md asks for maintainer agreement
before a new shared abstraction. #42054 creates that module.
- Split into two PRs. The two changes are one bug class in one option.
They are independent in the diff (`FetchTasklet.rs` and
`FetchSession.rs`), so a maintainer can ask for one half.

**Suites (debug build, ASAN).** `test/js/web/fetch/fetch.tls.test.ts`
(46 pass), `fetch-session.test.ts` (34),
`test/js/bun/http/proxy.test.ts` (92),
`proxy-stress-adversarial.test.ts` (151), `proxy-stress-errors.test.ts`
(53). On the same change before the last rebase:
`fetch.tls.ipv6.test.ts`, `test/regression/issue/26125.test.ts`,
`test/js/node/tls/fetch-tls-cert.test.ts`, `ssl-ctx-cache.test.ts`, the
`checkServerIdentity` test of `fetch-http2-client.test.ts`. The new
tests also pass with `BUN_JSC_validateExceptionChecks=1`.

</details>

<!-- robobun:evidence:begin -->

---

**no test proof** · iteration 0 · platform-specific test(s) that do not
run on this machine, deferring to CI, which covers all platforms:
test/js/web/fetch/fetch.tls.test.ts

<!-- robobun:evidence:end -->

… IP host (#42766)

### Problem

- `new Bun.SQL("postgres://u@[::1]:5432/db?sslmode=verify-full")`
rejects a certificate that has `IP:::1` with
`ERR_TLS_CERT_ALTNAME_INVALID`. `mysql://` does the same.
- `parseOptions` copies `URL.hostname`, brackets kept, into
`tls.serverName` (`src/js/internal/sql/shared.ts:2159`). Both adapters
check the certificate against that text, and `"[::1]"` is not an IP
address.
- Both adapters send an IP literal as SNI (`127.0.0.1`). RFC 6066
section 3 forbids that.

### Fix

- `SSLConfig::server_name_bytes()` (`src/sql_jsc/jsc.rs`) returns the
name through `bun_core::ip_address::strip_ipv6_brackets`. The identity
checks of both adapters read the name there.
- New `SSLConfig::sni()` returns no name for an IP literal, bracketed or
not. Both `adopt_tls` sites use it. libpq and fetch send none either.
- Verified: `test/js/sql/sql-tls-ip-literal-host.test.ts` (new, 8
cases). 6 fail on main 0d73249, all pass here.

### Background

- `sslmode=verify-full` checks that the certificate names the host. An
IP host matches only an IP SAN entry, and only as a bare address
(`check_x509_server_identity`, `src/boringssl/lib.rs:452`).
- SNI is the host name a TLS client sends in its first message.
`adopt_tls` sets it.
- Considered a strip in `parseOptions` (the first version). That was a
second copy of `strip_ipv6_brackets`, which fetch, RedisClient and
Bun.connect use, and it changed `sql.options`.

### Downsides

- Bun.SQL sends no SNI for an IP-literal name. A TLS proxy that routes
on such a value loses it.
- A zone-scoped address (`hostname: "::1%lo"`) still fails
`verify-full`, and without brackets it still goes out as SNI:
`is_ip_address` accepts no `%zone`.
- Cost per TLS connection: one `is_ip_address` call (at most 45 bytes,
no allocation) and a bracket check at each name read.

<details><summary>Notes</summary>

- History of this PR. The first version removed the brackets in
`parseOptions` (JS). Main then gained
`bun_core::ip_address::strip_ipv6_brackets` and moved every other client
to it. A review noted that the JS helper disagreed with it (`[db]` lost
its brackets too). aa4d699 moves the fix to the two native accessors
and restores `parseOptions` to its state on main. So this PR no longer
touches `src/js/internal/sql/shared.ts`, and it does not conflict with
#42054 or #41761, which edit that block.
- `sql.options.tls.serverName` and `sql.options.hostname` keep the
brackets, as on main. Only the native reads see the bare address.
- When this PR opened (canary 09bb546), Bun.SQL also accepted a
certificate whose only SAN is `DNS:[::1]`, and a hostname mismatch gave
an `Error` with an empty `code` and `message`. Main changed both since:
#43873 makes a name that is not a hostname match no certificate name,
and #43694 rejects the mismatch inside the handshake with
`ERR_TLS_CERT_ALTNAME_INVALID`. This PR changes neither.
- Probed on the debug build with a mock TLS server on 127.0.0.1 and an
explicit `tls.serverName`. `"[::1]"` and `"::1"` under `verify-full`:
connects, no SNI. `"[db]"` under `verify-full`:
`ERR_TLS_CERT_ALTNAME_INVALID`, as on main. `"[fe80::1%lo]"` under
`require`: no SNI. `"fe80::1%lo"` and `"127.1"` under `require`: sent as
SNI, because `is_ip_address` takes neither as an IP literal (#43979).
`"localhost"`: sent as SNI.
- Observed on main 0d73249: peer SNI `127.0.0.1` for host
`127.0.0.1`. With this change: none.
- The cases that dial `[::1]` gate on `isIPv6()` (Buildkite Linux has no
IPv6 loopback). The other cases run on every lane. One of them dials
127.0.0.1 with `tls.serverName: "[::1]"`, so every lane checks a
bracketed name against the `IP:::1` entry.
- Two gaps in the same `parseOptions` block are on main and stay out of
this PR: `tls.servername` (the Node spelling) is ignored, which #42054
owns, and `tls: true` without an sslmode derives no `serverName`, which
#26369 tracks. A `BunFile` given as `tls` with an explicit sslmode loses
the file there, which #41761 owns.
- Same bug class as #30668, which #30674 fixed for fetch and WebSocket.
The dial path already removes the brackets
(`src/uws_sys/socket.rs:791`).
- Other suites run on the merged debug build (main 601af5a):
`postgres-pgsslmode-env`, `sql-mysql-tls-plaintext-injection`, all of
`adapter-env-var-precedence`, and
`test/js/bun/net/tls-reject-before-client-cert.test.ts` (121 pass, 9
skip). I ran the new file 40 times on the earlier head: 320 of 320 cases
pass. The container TLS suites (`tls-sql`, `local-sql`) need Docker and
run in CI.

</details>

<!-- robobun:evidence:begin -->

---

**no test proof** · iteration 0 · platform-specific test(s) that do not
run on this machine, deferring to CI, which covers all platforms:
test/js/sql/adapter-env-var-precedence.test.ts

<!-- robobun:evidence:end -->

… accepts tls.servername (#42054)

RedisClient never set an SNI and matched the certificate against the URL host
only, so tls.serverName did nothing: a server behind an SNI router saw no name,
and a certificate for another name than the one dialed could not be accepted
or refused by name. One configured_hostname() (tls.serverName, else the URL
host) is now the SNI, unless it is an IP literal, and the name both checks
match. It replaces identity_hostname(), whose SSL_get_servername branch could
not be taken.

Bun.SQL filled in serverName over a tls.servername, the node:tls spelling that
the native SSLConfig already accepts.

tls.checkServerIdentity, the other half of #42054, is not part of this.

)

RedisClient's new SNI gate used the plain address check, so tls.serverName
"::1%lo" went out as a host name. It now uses the zone-aware check of the other
clients.

… accepts tls.servername (#42054)

RedisClient never set an SNI and matched the certificate against the URL host
only, so tls.serverName did nothing: a server behind an SNI router saw no name,
and a certificate for another name than the one dialed could not be accepted
or refused by name. One configured_hostname() (tls.serverName, else the URL
host) is now the SNI, unless it is an IP literal, and the name both checks
match. It replaces identity_hostname(), whose SSL_get_servername branch could
not be taken.

Bun.SQL filled in serverName over a tls.servername, the node:tls spelling that
the native SSLConfig already accepts.

tls.checkServerIdentity, the other half of #42054, is not part of this.

)

RedisClient's new SNI gate used the plain address check, so tls.serverName
"::1%lo" went out as a host name. It now uses the zone-aware check of the other
clients.